fix(tui): avoid hard-wrapping URL lines in history #8757
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
joshka-oai
left a comment
joshka-oai
left a comment
There was a problem hiding this comment.
I think we can go a bit deeper here and make an intelligent wrap before URLs a thing - i.e. when they'd wrap in the middle of the URL, force a wrap before. We will still need to character wrap when they're too long for the terminal regardless (and need tests for that scenario), but this would at least cut down the problems of not quite enough space to wrap. In effect, we want to treat the url as a word / non-breakable token somehow. This should probably also be in the wrapping code, not the insert_history code btw.
Strong support for fixing this. So please continue on this when you can.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
etraut-openai
added
needs-response
joshka-oai
left a comment
joshka-oai
left a comment
There was a problem hiding this comment.
When I tried this out in both tui and tui2, I asked codex to produce a url that was long enough to wrap. I saw the wrapping seemed like it still happened mid url. It would probably be good to add some tests that use insta around these things (as these tend to be visual rather than programmatic).
tui
tui2
| use ratatui::text::Line; | ||
| use ratatui::text::Span; | ||
|
|
||
| fn wrap_history_lines<'a>(lines: &'a [Line<'a>], width: u16) -> Vec<Line<'a>> { |
There was a problem hiding this comment.
needs a doc comment clarifying intent
| use ratatui::text::Line; | ||
| use ratatui::text::Span; | ||
|
|
||
| fn wrap_history_lines<'a>(lines: &'a [Line<'a>], width: u16) -> Vec<Line<'a>> { |
There was a problem hiding this comment.
nit (subjective preference): I'd prefer to see this below where it's called as it makes the code easier generally to read top-to-bottom outside-in.
| } else { | ||
| base_opts | ||
| .clone() | ||
| .initial_indent(base_opts.subsequent_indent.clone()) | ||
| }; |
There was a problem hiding this comment.
This seems a bit odd. I'm not sure I understand why we need to do this rather than relying on initial indent directly here.
| let width = width.max(1) as usize; | ||
| let mut rows = 0usize; | ||
| for line in lines { | ||
| let line_width = line.width().max(1); |
There was a problem hiding this comment.
I don't love the width() call here (it's likely not a significant perf hit, but becomes one easily with large enough history which can cause rendering slowdowns - more-so in tui2 than tui.
Not a big deal, but if there's a simple fix to avoid this (like storing the line + width computed once here) that would be great.
| for line in lines { | ||
| let line_width = line.width().max(1); | ||
| rows = rows.saturating_add(line_width.div_ceil(width)); | ||
| } |
There was a problem hiding this comment.
This can probably be more written more succinctly / obviously with map + sum
There was a problem hiding this comment.
Add covering tests (make codex produce these so that all the edge cases / behavior are properly tested).
3 participants
What
Why
How
Testing